feat: add security review skill to plan implementation#482
Open
dotuananh0712 wants to merge 3 commits intoobra:mainfrom
Open
feat: add security review skill to plan implementation#482dotuananh0712 wants to merge 3 commits intoobra:mainfrom
dotuananh0712 wants to merge 3 commits intoobra:mainfrom
Conversation
Adds guidance to run git diff commands separately instead of chaining with && to avoid consent prompts. Fixes obra#476
After generating a detailed implementation plan, clear context to free up memory for implementation and debugging. Fixes obra#478
- Enhanced code-reviewer.md with comprehensive security checklist - Added new security-reviewer.md template with OWASP Top 10 checks - Added new security-reviewer-prompt.md for dispatching security subagents - Updated subagent-driven-development workflow to include optional security review This addresses the issue that code review skill was not fully realized, and adds dedicated security review to the implementation process. Fixes obra#479
📝 WalkthroughWalkthroughThis PR integrates a dedicated security review stage into the development workflow, positioned after code quality checks. It includes new security review documentation with comprehensive checklists, improves existing code review guidance, adds memory management instructions for implementation, and updates the task workflow to dispatch security reviewers and handle security fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant Implementer
participant CodeQualityReviewer as Code Quality<br/>Reviewer
participant SecurityReviewer as Security<br/>Reviewer
participant TaskCompletion as Task<br/>Completion
Implementer->>CodeQualityReviewer: Submit implementation
CodeQualityReviewer->>CodeQualityReviewer: Review code quality
alt Code Quality Issues
CodeQualityReviewer-->>Implementer: Request changes
Implementer->>CodeQualityReviewer: Resubmit
else Code Quality Approved
CodeQualityReviewer->>SecurityReviewer: Dispatch security review
SecurityReviewer->>SecurityReviewer: Perform security review
alt Security Issues Found
SecurityReviewer-->>Implementer: Report vulnerabilities
Implementer->>Implementer: Fix security issues
Implementer->>SecurityReviewer: Resubmit for review
else Security Approved
SecurityReviewer->>TaskCompletion: Approve
TaskCompletion->>TaskCompletion: Complete task
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/subagent-driven-development/SKILL.md (1)
56-83:⚠️ Potential issue | 🟡 MinorFix DOT node/edge shapes for valid diagram semantics.
“Dispatch security reviewer” is an action (box), and
shapeon an edge is not valid DOT—this can break rendering or mislead readers.🧩 Suggested DOT fixes
- "Dispatch security reviewer subagent (./security-reviewer-prompt.md)?" [shape=diamond]; + "Dispatch security reviewer subagent (./security-reviewer-prompt.md)" [shape=box]; ... - "Dispatch security reviewer subagent (./security-reviewer-prompt.md)?" -> "Security reviewer subagent approves?" [shape=diamond]; + "Dispatch security reviewer subagent (./security-reviewer-prompt.md)" -> "Security reviewer subagent approves?";
🤖 Fix all issues with AI agents
In `@skills/executing-plans/SKILL.md`:
- Around line 23-25: The "Clear context" step may remove plan details created by
TodoWrite needed for Step 2; update SKILL.md to instruct preserving the plan
before clearing by saving the plan state (e.g., persist the TodoWrite output or
store a plan identifier) or explicitly reloading/reopening the plan after
clearing the window so Step 2 can access it; reference the TodoWrite action and
the Step 2 usage in the text and add a short sentence explaining how to
persist/reopen the plan (persist to storage or emit a plan token) immediately
before the "Tell Claude Code to clear its context/window" instruction.
In `@skills/requesting-code-review/security-reviewer.md`:
- Around line 45-51: Reword the "Input Validation:" checklist to remove
repetitive "No ..." starters for readability by changing each bullet to concise
negative-check phrases (e.g., "SQL injection risks?" -> "SQL injection risks
present?" or "Resistant to SQL injection?"; "command injection risks?" ->
"Resistant to command injection?"; "path traversal vulnerabilities?" -> "Path
traversal vulnerabilities present?"; "XSS prevention in place?" -> "XSS
prevention implemented?") while keeping the original intent and question format;
update the bullets under the "Input Validation" heading so they read
consistently and scan easily.
In `@skills/subagent-driven-development/SKILL.md`:
- Around line 194-200: The phrase "Two-stage review" under the "Quality gates:"
section is now incorrect; update the wording to "Three-stage review" and adjust
the bulleted sequence that currently reads "Two-stage review: spec compliance,
then code quality, then security (optional)" to explicitly reflect three stages
(spec compliance, code quality, security) and remove "(optional)" so it matches
the added security review; edit the "Quality gates:" block (the heading and its
bullets) to replace "Two-stage review" with "Three-stage review" and ensure the
bullets list the three stages in order.
|
That would be amazing tool! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This addresses the issue that code review skill was not fully realized,
and adds dedicated security review to the implementation process.
Fixes #479
Summary by CodeRabbit
New Features
Documentation